feat(jans-fido2): add controllers for metrics and resolve staging issues #12934
Conversation
Signed-off-by: imran <imranishaq7071@gmail.com>
📝 WalkthroughWalkthroughConvert metrics timestamps to UTC-backed Date types, add a new Fido2Metrics REST controller, align services and schedulers to UTC and cluster-aware execution, refactor metric/user persistence and mapping, update configuration to interval-based aggregation, and remove a trend-analysis unit test. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mo-auto
added
comp-jans-fido2
…ns-fido2-metrics-endpoints
Signed-off-by: imran <imranishaq7071@gmail.com>
|
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java (2)
63-71: Async execution uses common ForkJoinPool without bounds or timeout.Using
CompletableFuture.runAsync()without an explicit executor relies on the common ForkJoinPool. Under high metrics volume, this could:
- Exhaust pool threads, affecting other async operations in the JVM
- Provide no backpressure mechanism if persistence is slow
Consider using a dedicated bounded executor for metrics operations.
♻️ Proposed approach
// Add as class field private static final ExecutorService metricsExecutor = Executors.newFixedThreadPool(4, r -> { Thread t = new Thread(r, "fido2-metrics-worker"); t.setDaemon(true); return t; }); // Then use: CompletableFuture.runAsync(() -> { ... }, metricsExecutor);
337-339: Cleanup removes entries one-by-one, causing N+1 database operations.The current implementation fetches all entries, then removes them individually in a loop. For large retention cleanups, this is inefficient.
Consider using batch delete or a single delete-by-filter operation if the persistence layer supports it.
♻️ Proposed approach
// If PersistenceEntryManager supports batch removal: // persistenceEntryManager.removeByFilter(METRICS_ENTRY_BASE_DN, Fido2MetricsEntry.class, filter); // Or batch the removals: int batchSize = 100; for (int i = 0; i < entries.size(); i += batchSize) { List<Fido2MetricsEntry> batch = entries.subList(i, Math.min(i + batchSize, entries.size())); for (Fido2MetricsEntry entry : batch) { persistenceEntryManager.remove(entry); } }jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2UserMetrics.java (4)
96-102:transientfields with@JsonObjectannotation won't persist correctly.The
userSegmentsandbehaviorPatternsfields are marked astransient, which prevents Java serialization. However, they also have@JsonObjectannotation for ORM persistence. This combination may cause data loss during serialization/deserialization cycles.If these fields need ORM persistence, remove
transient. If they are meant to be computed/transient, remove@JsonObject.🔧 Suggested fix (if persistence is intended)
`@AttributeName`(name = "jansUserSegments") `@JsonObject` - private transient Map<String, Object> userSegments; + private Map<String, Object> userSegments; `@AttributeName`(name = "jansBehaviorPatterns") `@JsonObject` - private transient Map<String, Object> behaviorPatterns; + private Map<String, Object> behaviorPatterns;
354-380: Minor timestamp inconsistency: twonew Date()calls per increment.Each increment method creates two separate
Dateinstances forlastActivityDateandlastUpdated. While the time difference is negligible, using a single timestamp would be more consistent and slightly more efficient.♻️ Suggested improvement
public void incrementRegistrations(boolean success) { this.totalRegistrations++; if (success) { this.successfulRegistrations++; } else { this.failedRegistrations++; } - this.lastActivityDate = new Date(); - this.lastUpdated = new Date(); + Date now = new Date(); + this.lastActivityDate = now; + this.lastUpdated = now; }Apply similar pattern to
incrementAuthenticationsandincrementFallbackEvents.
473-487: Consider usingidfield for equality instead of mutable fields.The
equalsandhashCodemethods usefirstRegistrationDate(a mutableDate), which is risky if instances are stored in hash-based collections. For ORM entities, using the primary identifier (id) is typically safer and more predictable.♻️ Suggested fix
`@Override` public boolean equals(Object obj) { if (this == obj) return true; if (obj == null || getClass() != obj.getClass()) return false; Fido2UserMetrics that = (Fido2UserMetrics) obj; - return Objects.equals(userId, that.userId) && - Objects.equals(username, that.username) && - Objects.equals(firstRegistrationDate, that.firstRegistrationDate); + return Objects.equals(id, that.id); } `@Override` public int hashCode() { - return Objects.hash(userId, username, firstRegistrationDate); + return Objects.hash(id); }
33-114: Add property descriptions for API documentation.The fields lack descriptions that would be reflected in auto-generated Swagger specifications. Consider adding
@Schemaor similar annotations with descriptions for fields likeuserId,username,riskScore,engagementLevel, etc., to improve API documentation quality. Based on learnings, missing property descriptions should be raised in components likejans-fido2.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2UserMetricsService.java (3)
69-96: Fire-and-forget async without error propagation or timeout.
CompletableFuture.runAsync()without a custom executor uses the common ForkJoinPool and has no timeout. If the persistence layer is slow or blocked, tasks accumulate. Also, exceptions are only logged but not propagated or monitored.Consider:
- Using a bounded executor to prevent resource exhaustion
- Adding timeouts via
orTimeout()orcompleteOnTimeout()- Monitoring failed futures for observability
♻️ Example with bounded executor and timeout
// At class level: private static final ExecutorService metricsExecutor = Executors.newFixedThreadPool( Runtime.getRuntime().availableProcessors(), r -> { Thread t = new Thread(r, "fido2-user-metrics"); t.setDaemon(true); return t; } ); // In method: CompletableFuture.runAsync(() -> { // ... existing logic }, metricsExecutor) .orTimeout(30, TimeUnit.SECONDS) .exceptionally(ex -> { log.error("User metrics update failed: {}", ex.getMessage()); return null; });
245-260: Simplify UTC date conversion ingetNewUsers.The conversion chain
ZonedDateTime -> LocalDateTime -> ZonedDateTime -> Instant -> Dateis unnecessarily complex.♻️ Simplified conversion
public List<Fido2UserMetrics> getNewUsers(int days) { try { - // Use UTC timezone to align with FIDO2 services - LocalDateTime cutoffLdt = ZonedDateTime.now(ZoneId.of("UTC")) - .toLocalDateTime() - .minusDays(days); - Date cutoffDate = Date.from(cutoffLdt.atZone(ZoneId.of("UTC")).toInstant()); + Date cutoffDate = Date.from( + ZonedDateTime.now(ZoneId.of("UTC")) + .minusDays(days) + .toInstant() + ); Filter filter = Filter.createGreaterOrEqualFilter("jansFirstRegistrationDate", cutoffDate);
463-488: Integer division loses precision in average duration calculation.The moving average calculation uses
longdivision which truncates fractional results, accumulating error over time. For metrics where precision matters, consider maintaining a running sum or using floating-point arithmetic.🔧 Alternative: Use double for intermediate calculation
} else { // Simple moving average - in production, you might want more sophisticated calculation long currentAvg = userMetrics.getAvgRegistrationDuration(); int count = userMetrics.getSuccessfulRegistrations() != null ? userMetrics.getSuccessfulRegistrations() : 1; - long newAvg = (currentAvg * (count - 1) + durationMs) / count; + long newAvg = Math.round((currentAvg * (count - 1.0) + durationMs) / count); userMetrics.setAvgRegistrationDuration(newAvg); }jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (1)
166-175: Async metric recording uses unbounded common pool.Same pattern as in
Fido2UserMetricsService-CompletableFuture.runAsync()without a custom executor may lead to thread pool exhaustion under high load. Consider using a dedicated bounded executor for metrics operations.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2AnalyticsService.java (1)
46-62: Consider extracting magic numbers to configuration.While some thresholds are loaded from
ResourceBundle, several magic numbers remain hardcoded (Lines 54-61):LOW_SUCCESS_RATE_THRESHOLD = 0.8,HIGH_SUCCESS_RATE_THRESHOLD = 0.95, authentication benchmark thresholds (Lines 631-639), etc.For consistency, consider moving all thresholds to the
fido2-metrics.propertiesconfiguration file.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java (1)
293-313: Verify that aggregation methods implement idempotent persistence or database-level unique constraints.The aggregation methods (
createHourlyAggregation,createDailyAggregation, etc.) callpersistenceEntryManager.persist()without checking if an aggregation already exists for the period. If multiple nodes experience cluster coordination failures and all fall back to single-node behavior, they will attempt to create duplicate aggregation records.Unlike
Fido2UserMetricsService, which properly checks existence and usesmerge()for updates, the aggregation methods lack this upsert pattern. The exception handlers only log errors without preventing duplicates. Additionally,Fido2MetricsAggregationhas no visible@Uniqueor@UniqueConstraintannotations to enforce database-level uniqueness.Either implement existence checks before persistence (following the pattern in
Fido2UserMetricsServicelines 428–436) or add database constraints to the aggregation ID.
🤖 Fix all issues with AI agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java`:
- Line 17: Remove the unused import of java.time.LocalDateTime from the
Fido2MetricsAggregation class; open the file containing class
Fido2MetricsAggregation and delete the import line referencing LocalDateTime
since the class now uses java.util.Date types and the LocalDateTime symbol is no
longer referenced.
- Around line 69-76: The Fido2MetricsAggregation(String aggregationType, String
period, Date startTime, Date endTime) constructor sets lastUpdated via new
Date() which uses system default timezone; change it to create a UTC-based Date
(e.g., assign lastUpdated = Date.from(Instant.now()) or
Date.from(ZonedDateTime.now(ZoneId.of("UTC")).toInstant())) so lastUpdated is
consistently UTC; update imports as needed and keep the assignment located in
the same constructor.
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsData.java`:
- Around line 105-106: Fido2MetricsData currently sets timestamp as
LocalDateTime (ZonedDateTime.now(ZoneId.of("UTC")).toLocalDateTime()), while
Fido2MetricsAggregation uses java.util.Date; to make models consistent, change
Fido2MetricsData to store timestamps as Date (UTC) instead of LocalDateTime and
initialize it using Date.from(Instant.now()) or equivalent in the constructor;
update any getters/setters in Fido2MetricsData to use Date and ensure the
service layer method convertToDate() is no longer required for this conversion
(or adjust its usage) so both Fido2MetricsData and Fido2MetricsAggregation share
the same timestamp type.
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java`:
- Around line 90-92: The field additionalData in class Fido2MetricsEntry is
marked transient while annotated with `@JsonObject/`@AttributeName, so it will not
be persisted; remove the transient modifier (or otherwise make it non-transient)
on the additionalData field so the ORM can serialize it, ensuring the
`@JsonObject` annotation on additionalData is effective; update the declaration of
additionalData in Fido2MetricsEntry (matching the pattern used in
Fido2UserMetrics) and run tests to confirm persistence.
- Around line 38-98: Add an LDAP schema definition for the new Fido2 metrics
model: create an objectClass named jansFido2MetricsEntry and define all
jansFido2Metrics* attributes used by the Fido2MetricsEntry class (e.g.,
jansFido2MetricsId, jansFido2MetricsType, jansFido2MetricsTimestamp,
jansFido2MetricsUserId, jansFido2MetricsUsername, jansFido2MetricsOperationType,
jansFido2MetricsStatus, jansFido2MetricsDuration,
jansFido2MetricsAuthenticatorType, jansFido2MetricsDeviceInfo,
jansFido2MetricsErrorReason, jansFido2MetricsErrorCategory,
jansFido2MetricsFallbackMethod, jansFido2MetricsFallbackReason,
jansFido2MetricsUserAgent, jansFido2MetricsIpAddress, jansFido2MetricsSessionId,
jansFido2MetricsAdditionalData, jansFido2MetricsNodeId,
jansFido2MetricsApplicationType) in the LDAP LDIF schema (the same template
location used for other FIDO2 schemas); specify appropriate syntaxes (e.g.,
distinguishedName/string/Date/Integer/JSON where supported), single- vs
multi-valued constraints, and include the new jansFido2MetricsEntry objectClass
in the appropriate structural/auxiliary category so instances of the
Fido2MetricsEntry Java class can be created and validated by the directory.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java`:
- Around line 164-204: Track non-fatal timer init failures by adding a boolean
or counter (e.g., failedTimerInits or hasTimerInitWarnings) in AppInitializer
and increment/set it inside each catch block for metricService.initTimer,
configurationFactory.initTimer, loggerService.initTimer, cleanerTimer.initTimer,
mds3UpdateTimer.initTimer, and customScriptManager.initTimer; after all
try/catch blocks, change the final success logging to check this flag/counter
and log either a normal success or a warning like "Completed with warnings: X
timer(s) failed to initialize" (include the count) so the final message reflects
partial failures.
- Around line 206-216: The null check on the injected CDI Instance
fido2MetricsAggregationSchedulerInstance is redundant because CDI always injects
the wrapper; remove the "fido2MetricsAggregationSchedulerInstance != null &&"
portion and only check isUnsatisfied() before calling
fido2MetricsAggregationSchedulerInstance.get().initTimer(), keeping the existing
try/catch and log messages (references:
fido2MetricsAggregationSchedulerInstance, isUnsatisfied(), get(), initTimer()).
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java`:
- Around line 106-119: The warning currently logs a hardcoded failure reason
when scheduler.startPeriodicLockUpdates() returns null; replace the misleading
literal with a generic message that does not invent the cause. Update the
log.warn call (in Fido2MetricsAggregationScheduler around updateTask handling)
to something like "Cluster lock updates unavailable, running in single-node mode
for {}" using jobType, or otherwise surface a real error if
startPeriodicLockUpdates() is changed to return/throw the actual failure; remove
the hardcoded "ou=node is not configured..." text so the log reflects only known
facts.
- Around line 419-434: The initializeClusterEnvironment method currently catches
a generic Exception when attempting
CDI.current().select(Fido2ClusterNodeService.class).get(), which hides whether
the failure is simply UnsatisfiedResolutionException (no bean) or a real
configuration/runtime error; update the try/catch to catch
javax.enterprise.inject.UnsatisfiedResolutionException specifically to handle
the single-node case (set isClusterEnvironment=false, updateExecutor=null and
log that cluster service is absent), and add a separate catch for Exception (or
Throwable) to log the full error and rethrow or handle as a true failure so
configuration errors when obtaining clusterNodeService are visible; keep
references to clusterNodeService, updateExecutor, isClusterEnvironment and the
logging messages in Fido2MetricsAggregationScheduler.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java`:
- Around line 323-329: The current Fido2MetricsService code performs an
unnecessary ZonedDateTime → LocalDateTime → ZonedDateTime → Date conversion for
cutoffDate; replace this with a direct Instant-based computation: compute an
Instant by subtracting retentionDays from Instant.now(Clock.systemUTC()) (or
Instant.now().minus(retentionDays, ChronoUnit.DAYS)) and then create cutoffDate
via Date.from(instant), updating references to cutoffDateTime/cutoffDate
accordingly to remove the redundant LocalDateTime conversion.
- Around line 647-653: The helper convertToDate(LocalDateTime dateTime)
currently assumes the input is UTC without stating or enforcing it; update the
method to either rename it to convertToDateUtc or change its signature to accept
an explicit ZoneId (e.g., convertToDate(LocalDateTime dateTime, ZoneId zone))
and use that zone when calling dateTime.atZone(zone).toInstant(); update all
callers in Fido2MetricsService to pass ZoneId.of("UTC") (or the appropriate
zone) and add a one-line Javadoc clarifying the required timezone behavior if
you choose the rename approach.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java`:
- Around line 470-512: The extractIpAddress method currently trusts
client-supplied proxy headers; update it so headers are only used when the
request came through a trusted proxy: add a configurable flag and trusted-proxy
list (e.g., via an injected AppConfiguration or MetricService constructor),
check request.getRemoteAddr() is in that trusted-proxy set (or
appConfiguration.isBehindTrustedProxy() + isTrustedProxy(remoteAddr)) before
reading X-Forwarded-For/other headers in extractIpAddress(HttpServletRequest
request), otherwise always return request.getRemoteAddr(); ensure the
trusted-proxy config is plumbed into MetricService and documented in the config
object so header-trusting is opt-in and configurable.
- Around line 521-563: getUserIdFromUsername currently issues a persistence
query on every call (using userPersistenceEntryManager) which can overload DB
under high throughput; change MetricService to first consult a threadsafe
username->userId cache (e.g., userIdCache in MetricService) with TTL and only
call the existing persistence lookup code when cache miss, storing resolved inum
in the cache (and caching negative/fallback results appropriately), and add an
optional configuration flag (e.g., enableUserLookup) so the lookup can be
disabled; refactor the persistence logic into a helper like
lookupUserIdFromPersistence(username) that contains the current
findEntries/filter/inum resolution so getUserIdFromUsername only handles cache,
TTL eviction, and optional config checks.
- Around line 599-612: Replace direct username logging in MetricService where
log.debug uses metricsData.getUsername() (the three occurrences after
updateUserRegistrationMetrics, updateUserAuthenticationMetrics, and
updateUserFallbackMetrics) with a privacy-safe identifier: either log the
userId/inum (e.g., request.getUserId() or metricsData.getUserId()) or a
deterministic hash/anonymized token of the username; ensure the same identifier
is used consistently across the three branches so logs remain correlatable but
free of raw PII, and update log messages to reflect the new field (e.g.,
"Updated user registration metrics for userId:{}" or "for userHash:{}").
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`:
- Around line 56-57: The ISO_FORMATTER currently set to
DateTimeFormatter.ISO_LOCAL_DATE_TIME in Fido2MetricsController rejects ISO-8601
strings with offsets; update the parsing/formatting to accept offset/zulu
timestamps by replacing or augmenting ISO_FORMATTER handling with an
offset-aware flow: attempt parsing with DateTimeFormatter.ISO_OFFSET_DATE_TIME
(or OffsetDateTime.parse) and convert the result to UTC (e.g., to Instant or
OffsetDateTime.withOffsetSameInstant(ZoneOffset.UTC) then to
LocalDateTime/desired format), and fallback to the existing ISO_LOCAL_DATE_TIME
parse if needed; ensure all places referencing ISO_FORMATTER or parsing
timestamps in Fido2MetricsController use this new accept-and-normalize logic and
update any API docs/comments to indicate offsets are supported.
- Around line 362-377: The controller returns a raw Map from the /config
endpoint in Fido2MetricsController which yields poor Swagger docs; create a new
DTO class (e.g., Fido2MetricsConfigResponse) with fields matching the map
entries (metricsEnabled, aggregationEnabled, retentionDays,
deviceInfoCollection, errorCategorization, performanceMetrics,
supportedAggregationTypes) and annotate each field with
`@Schema`(description="...") and proper types, then update the controller to
construct and return that DTO (instead of the Map and
dataMapperService.writeValueAsString(config)) so the endpoint method signature
exposes the concrete response type for OpenAPI generation.
- Around line 464-477: In processRequest (Fido2MetricsController.processRequest)
avoid including e.getMessage() in the client-facing
errorResponseFactory.unknownError; instead log the full exception with log.error
(already present) and pass a generic, non-sensitive message to unknownError
(e.g., "An unexpected error occurred while processing the metrics request") or a
sanitized summary; optionally generate and return an error id/UUID to the client
and include that id in the server log alongside the exception to correlate logs
(use that id in the unknownError payload) so internal details never leak.
- Around line 385-398: The getHealth method currently always returns status
"UP"; update it to perform a simple connectivity check via the metricsService
(e.g., call a lightweight method like metricsService.isHealthy() or
metricsService.ping()/metricsService.testConnection()) and catch any exceptions
from that call, setting "status" to "DOWN" and including an error message when
the check fails; retain the existing fields (metricsEnabled, aggregationEnabled,
timestamp) and ensure you still return the serialized map via
dataMapperService.writeValueAsString(health) while handling exceptions so
failures produce an informative JSON health response instead of always "UP".
- Around line 37-39: Fido2MetricsController currently exposes user-specific
endpoints like /entries/user/{userId} without auth; update
Fido2MetricsController to enforce authorization by adding an appropriate
security annotation (e.g., `@ProtectedApi` or `@RolesAllowed`("ROLE_ADMIN") or
similar used across the project) on the class or on the user-specific method(s)
that handle /entries/user/{userId}, or if security is handled outside the app,
add a clear comment/Javadoc on Fido2MetricsController stating that
authentication/authorization is enforced by the API gateway/OAuth interceptor
and specify the required role/scope so reviewers know the endpoint is protected.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.javajans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsConstants.javajans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsData.javajans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.javajans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2UserMetrics.javajans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.javajans-fido2/server/src/main/java/io/jans/fido2/service/app/ResteasyInitializer.javajans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2AnalyticsService.javajans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.javajans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.javajans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2UserMetricsService.javajans-fido2/server/src/main/java/io/jans/fido2/service/shared/LoggerService.javajans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.javajans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.javajans-fido2/server/src/main/resources/fido2-metrics.propertiesjans-fido2/server/src/test/java/io/jans/fido2/service/metric/Fido2MetricsTrendAnalysisTest.java
💤 Files with no reviewable changes (2)
- jans-fido2/server/src/main/java/io/jans/fido2/service/shared/LoggerService.java
- jans-fido2/server/src/test/java/io/jans/fido2/service/metric/Fido2MetricsTrendAnalysisTest.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pujavs
Repo: JanssenProject/jans PR: 12847
File: jans-config-api/docs/jans-config-api-swagger.yaml:15497-15498
Timestamp: 2025-12-17T13:07:08.750Z
Learning: When reviewing components like `jans-auth`, `jans-fido2`, `jans-lock` in the JanssenProject/jans repository, raise issues about missing property descriptions in API model classes, as these descriptions will be reflected in the auto-generated Swagger specifications.
🧬 Code graph analysis (3)
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java (1)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsConstants.java (1)
Fido2MetricsConstants(15-121)
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java (1)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsConstants.java (1)
Fido2MetricsConstants(15-121)
jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (1)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/UserMetricsUpdateRequest.java (1)
UserMetricsUpdateRequest(17-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: sonar scan (jans-linux-setup)
- GitHub Check: sonar scan (jans-pycloudlib)
- GitHub Check: sonar scan (jans-core)
- GitHub Check: sonar scan (jans-auth-server)
- GitHub Check: sonar scan (jans-keycloak-integration)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: sonar scan (jans-link)
- GitHub Check: sonar scan (jans-config-api)
- GitHub Check: sonar scan (jans-fido2)
- GitHub Check: sonar scan (jans-cli-tui)
- GitHub Check: sonar scan (jans-scim)
🔇 Additional comments (22)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsConstants.java (2)
74-74: LGTM!The renamed constant
jansFido2MetricsTimestampprovides a more specific and descriptive attribute name, distinguishing FIDO2 metrics timestamps from other timestamp fields in the system.
111-113: LGTM!The new query parameter constants support the REST API's time-based filtering requirements and align with the PR objectives.
jans-fido2/server/src/main/resources/fido2-metrics.properties (1)
1-32: Configuration structure is well-organized.The staggered execution times (5, 10, 15, 20 minutes past the hour) help prevent concurrent aggregation runs. The retention periods are reasonable.
Consider documenting the timezone assumption for cron expressions. Since the service uses UTC for timestamps, the cron jobs should ideally also execute in UTC context to ensure consistency. This may need to be configured at the scheduler level.
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java (1)
276-294: Defensive numeric conversion is appropriate.The updated
getMapMetric()correctly handles JSON deserialization edge cases where numeric values may be deserialized asIntegerinstead ofLong. The explicit conversion via((Number) val).longValue()is a robust solution.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java (4)
91-98: UTC conversion for persistence filters is correct.The
convertToDate()helper properly convertsLocalDateTimetoDateusing UTC zone, aligning with the broader UTC strategy in this PR.
296-302: Interval overlap logic is correctly implemented.The filter correctly identifies aggregations that overlap with the query range using the standard interval overlap condition:
aggregation.startTime <= queryEndTime AND aggregation.endTime >= queryStartTime.
693-698: Essential fields may include null values.The
setEssentialFieldsmethod setsuserId,username,operationType, andstatusunconditionally, but these fields may be null in the sourcemetricsData. If null values are problematic for persistence, consider adding null guards similar tosetOptionalFields.
743-761: Device info population logic is well-structured.The boolean accumulation pattern (
hasDeviceInfo |= setDeviceField(...)) correctly tracks whether any device field was set, avoiding persisting emptyDeviceInfoobjects.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2UserMetricsService.java (2)
335-378: LGTM - UTC-based behavior pattern analysis.The method correctly uses UTC for temporal calculations and provides comprehensive user behavior analytics.
425-440: LGTM - Proper persist/merge distinction for user metrics.The method correctly differentiates between new entries (using
persist) and existing entries (usingmerge) based on DN presence.jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (1)
349-408: LGTM - Robust metrics data creation with graceful degradation.The method implements defensive coding with multiple try-catch blocks, ensuring partial failures (device info extraction, node ID retrieval) don't prevent metric collection. Good use of fallback to minimal device info on extraction failure.
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java (2)
105-111: LGTM - UTC timestamp initialization in constructor.The constructor correctly initializes the timestamp using UTC timezone, consistent with the broader UTC standardization in this PR.
33-34: Good addition:@JsonInclude(NON_NULL)optimizes JSON responses.This annotation reduces API response payload size by excluding null fields, which aligns with the PR objective of optimizing JSON responses.
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2AnalyticsService.java (2)
92-98: LGTM - UTC-aligned report metadata.The report metadata now correctly uses UTC for the
generatedAttimestamp, ensuring consistency across the metrics system.
557-587: LGTM - Correct UTC conversion for seasonal pattern analysis.The method properly converts
DatetoLocalDateTimeusing UTC zone before extracting day-of-week information, maintaining consistency with the UTC-based timestamp approach across the PR.jans-fido2/server/src/main/java/io/jans/fido2/service/app/ResteasyInitializer.java (1)
37-37: LGTM!The
Fido2MetricsControllerregistration follows the existing pattern for exposing REST controllers and properly integrates the new metrics endpoints.jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java (1)
107-108: LGTM: Proper CDI pattern for optional bean.Using
Instance<Fido2MetricsAggregationScheduler>allows lazy access and availability checking, which is appropriate for an optional feature component.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java (4)
72-86: Acceptable defensive loading for optional configuration.The static block gracefully handles missing
fido2-metrics.propertiesfile with a fallback to defaults. This ensures the scheduler can operate even without a configuration file.
147-155: LGTM: Consistent UTC handling across all aggregation jobs.The UTC-based timestamp calculations are correct and consistent across hourly, daily, weekly, and monthly aggregations. This aligns with the broader FIDO2 services UTC convention mentioned in the AI summary.
371-412: Lock update mechanism is sound.The periodic lock update logic correctly handles both cluster and single-node modes. The 30-second refresh interval is reasonable for maintaining lock heartbeats.
Consider making the refresh interval configurable if cluster environments have varying latency requirements.
546-562: LGTM: Proper executor shutdown handling.The shutdown logic follows best practices: graceful shutdown with timeout, forced shutdown on timeout, and proper handling of
InterruptedExceptionby restoring the interrupt flag.jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java (1)
443-457: LGTM: Robust aggregation type validation.The validation correctly normalizes to uppercase before comparison and provides a clear error message listing valid options.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import io.jans.orm.model.base.Entry; | ||
|
|
||
| import java.io.Serializable; | ||
| import java.time.LocalDateTime; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import after migration.
The LocalDateTime import appears unused after the migration to Date types. Consider removing it to keep the imports clean.
🔧 Proposed fix
import java.io.Serializable;
-import java.time.LocalDateTime;
import java.util.Collections;
import java.util.Date;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import java.time.LocalDateTime; | |
| import java.io.Serializable; | |
| import java.util.Collections; | |
| import java.util.Date; |
🤖 Prompt for AI Agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java`
at line 17, Remove the unused import of java.time.LocalDateTime from the
Fido2MetricsAggregation class; open the file containing class
Fido2MetricsAggregation and delete the import line referencing LocalDateTime
since the class now uses java.util.Date types and the LocalDateTime symbol is no
longer referenced.
| public Fido2MetricsAggregation(String aggregationType, String period, Date startTime, Date endTime) { | ||
| this(); | ||
| this.aggregationType = aggregationType; | ||
| this.id = aggregationType + "_" + period; | ||
| this.startTime = startTime; | ||
| this.endTime = endTime; | ||
| this.lastUpdated = LocalDateTime.now(); | ||
| this.lastUpdated = new Date(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider UTC consistency for lastUpdated initialization.
The constructor uses new Date() for lastUpdated, which uses the system default timezone. Other parts of the PR explicitly use UTC (e.g., ZonedDateTime.now(ZoneId.of("UTC"))). For consistency, consider initializing with an explicit UTC-based date.
♻️ Proposed fix
+import java.time.Instant;
...
public Fido2MetricsAggregation(String aggregationType, String period, Date startTime, Date endTime) {
this();
this.aggregationType = aggregationType;
this.id = aggregationType + "_" + period;
this.startTime = startTime;
this.endTime = endTime;
- this.lastUpdated = new Date();
+ this.lastUpdated = Date.from(Instant.now()); // Instant.now() is always UTC
}🤖 Prompt for AI Agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java`
around lines 69 - 76, The Fido2MetricsAggregation(String aggregationType, String
period, Date startTime, Date endTime) constructor sets lastUpdated via new
Date() which uses system default timezone; change it to create a UTC-based Date
(e.g., assign lastUpdated = Date.from(Instant.now()) or
Date.from(ZonedDateTime.now(ZoneId.of("UTC")).toInstant())) so lastUpdated is
consistently UTC; update imports as needed and keep the assignment located in
the same constructor.
| // Use UTC timezone to align with existing FIDO2 services | ||
| this.timestamp = ZonedDateTime.now(ZoneId.of("UTC")).toLocalDateTime(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
UTC initialization is correct, but consider consistency with other models.
The UTC-aligned timestamp initialization is appropriate. However, there's an inconsistency: Fido2MetricsAggregation uses Date types for timestamps while this class uses LocalDateTime. The service layer's convertToDate() method handles the conversion, but consider aligning the timestamp types across models for long-term maintainability.
🤖 Prompt for AI Agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsData.java`
around lines 105 - 106, Fido2MetricsData currently sets timestamp as
LocalDateTime (ZonedDateTime.now(ZoneId.of("UTC")).toLocalDateTime()), while
Fido2MetricsAggregation uses java.util.Date; to make models consistent, change
Fido2MetricsData to store timestamps as Date (UTC) instead of LocalDateTime and
initialize it using Date.from(Instant.now()) or equivalent in the constructor;
update any getters/setters in Fido2MetricsData to use Date and ensure the
service layer method convertToDate() is no longer required for this conversion
(or adjust its usage) so both Fido2MetricsData and Fido2MetricsAggregation share
the same timestamp type.
| @AttributeName(name = "jansFido2MetricsId") | ||
| private String id; | ||
|
|
||
| @AttributeName(name = "jansMetricType") | ||
| @AttributeName(name = "jansFido2MetricsType") | ||
| private String metricType; | ||
|
|
||
| @AttributeName(name = "jansTimestamp") | ||
| private LocalDateTime timestamp; | ||
| @AttributeName(name = "jansFido2MetricsTimestamp") | ||
| private Date timestamp; | ||
|
|
||
| @AttributeName(name = "jansUserId") | ||
| @AttributeName(name = "jansFido2MetricsUserId") | ||
| private String userId; | ||
|
|
||
| @AttributeName(name = "jansUsername") | ||
| @AttributeName(name = "jansFido2MetricsUsername") | ||
| private String username; | ||
|
|
||
| @AttributeName(name = "jansOperationType") | ||
| @AttributeName(name = "jansFido2MetricsOperationType") | ||
| private String operationType; // REGISTRATION, AUTHENTICATION, FALLBACK | ||
|
|
||
| @AttributeName(name = "jansStatus") | ||
| @AttributeName(name = "jansFido2MetricsStatus") | ||
| private String status; // SUCCESS, FAILURE, ATTEMPT | ||
|
|
||
| @AttributeName(name = "jansDurationMs") | ||
| @AttributeName(name = "jansFido2MetricsDuration") | ||
| private Long durationMs; | ||
|
|
||
| @AttributeName(name = "jansAuthenticatorType") | ||
| @AttributeName(name = "jansFido2MetricsAuthenticatorType") | ||
| private String authenticatorType; // PLATFORM, CROSS_PLATFORM, SECURITY_KEY | ||
|
|
||
| @AttributeName(name = "jansDeviceInfo") | ||
| @AttributeName(name = "jansFido2MetricsDeviceInfo") | ||
| @JsonObject | ||
| private DeviceInfo deviceInfo; | ||
|
|
||
| @AttributeName(name = "jansErrorReason") | ||
| @AttributeName(name = "jansFido2MetricsErrorReason") | ||
| private String errorReason; | ||
|
|
||
| @AttributeName(name = "jansErrorCategory") | ||
| @AttributeName(name = "jansFido2MetricsErrorCategory") | ||
| private String errorCategory; | ||
|
|
||
| @AttributeName(name = "jansFallbackMethod") | ||
| @AttributeName(name = "jansFido2MetricsFallbackMethod") | ||
| private String fallbackMethod; | ||
|
|
||
| @AttributeName(name = "jansFallbackReason") | ||
| @AttributeName(name = "jansFido2MetricsFallbackReason") | ||
| private String fallbackReason; | ||
|
|
||
| @AttributeName(name = "jansUserAgent") | ||
| @AttributeName(name = "jansFido2MetricsUserAgent") | ||
| private String userAgent; | ||
|
|
||
| @AttributeName(name = "jansIpAddress") | ||
| @AttributeName(name = "jansFido2MetricsIpAddress") | ||
| private String ipAddress; | ||
|
|
||
| @AttributeName(name = "jansSessionId") | ||
| @AttributeName(name = "jansFido2MetricsSessionId") | ||
| private String sessionId; | ||
|
|
||
| @AttributeName(name = "jansAdditionalData") | ||
| @AttributeName(name = "jansFido2MetricsAdditionalData") | ||
| @JsonObject | ||
| private transient Map<String, Object> additionalData; | ||
|
|
||
| @AttributeName(name = "jansNodeId") | ||
| @AttributeName(name = "jansFido2MetricsNodeId") | ||
| private String nodeId; | ||
|
|
||
| @AttributeName(name = "jansApplicationType") | ||
| @AttributeName(name = "jansFido2MetricsApplicationType") | ||
| private String applicationType; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file to see the actual attribute names being used
cat -n jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java | sed -n '30,110p'Repository: JanssenProject/jans
Length of output: 3270
🏁 Script executed:
# Search for LDAP schema or migration-related files
find . -type f \( -name "*.ldif" -o -name "*.schema" -o -name "*migration*" -o -name "*upgrade*" \) | grep -i fido 2>/dev/null | head -20Repository: JanssenProject/jans
Length of output: 330
🏁 Script executed:
# Check for references to old Fido2Metrics attribute names in the codebase
rg "jansMetric|jansFido2Metric" --type java -c | head -20Repository: JanssenProject/jans
Length of output: 561
🏁 Script executed:
# Look for git history or comments about migration in related files
find . -path "*fido2*" -name "*.java" | xargs grep -l "migration\|deprecated\|backwards.*compat" 2>/dev/null | head -10Repository: JanssenProject/jans
Length of output: 45
🏁 Script executed:
# Check the broader LDAP schema context for Fido2 attributes
find . -type f \( -name "*.ldif" -o -name "*.schema" \) | xargs grep -l "fido" -i 2>/dev/nullRepository: JanssenProject/jans
Length of output: 532
🏁 Script executed:
# Check the LDIF schema file for jansFido2Metrics attributes
cat ./jans-linux-setup/jans_setup/templates/jans-fido2/fido2.ldif | grep -A 50 -i "jansFido2Metrics"Repository: JanssenProject/jans
Length of output: 45
🏁 Script executed:
# Check the upgrade.py script for migration logic
cat ./docker-jans-fido2/scripts/upgrade.py | grep -i "metric\|attribute\|migration" -C 3Repository: JanssenProject/jans
Length of output: 603
🏁 Script executed:
# Search for references to the old MetricEntry class to understand the old schema
cat -n ./jans-core/model/src/main/java/io/jans/model/metric/ldap/MetricEntry.java | head -60Repository: JanssenProject/jans
Length of output: 1964
🏁 Script executed:
# Check if there are test data files that show the old vs new attribute names
rg "MetricsEntry|metricType|operationType" --type ldif | head -20Repository: JanssenProject/jans
Length of output: 91
🏁 Script executed:
# Look for any comments or documentation about the metrics schema
find . -path "*/fido2*" -type f \( -name "*.md" -o -name "*.txt" -o -name "README*" \) | xargs grep -l "metric" -i 2>/dev/nullRepository: JanssenProject/jans
Length of output: 313
🏁 Script executed:
# Read the LDIF schema file for FIDO2
cat ./jans-linux-setup/jans_setup/templates/jans-fido2/fido2.ldifRepository: JanssenProject/jans
Length of output: 300
🏁 Script executed:
# Search for any historical references or old names of Fido2MetricsEntry
rg "MetricsEntry|Fido2Metric" --type java -B 2 -A 5 | head -50Repository: JanssenProject/jans
Length of output: 5567
🏁 Script executed:
# Check if the test data LDIF files reference the new attribute names
cat ./jans-linux-setup/jans_setup/templates/test/jans-fido2/data/fido2-device-registration-test-data.ldif | head -100Repository: JanssenProject/jans
Length of output: 1103
🏁 Script executed:
# Look for any version or changelog information about Fido2MetricsEntry
find . -path "*/fido2*" -type f \( -name "CHANGELOG*" -o -name "*.md" -o -name "*.txt" \) | xargs grep -l "MetricsEntry\|metrics" 2>/dev/null | head -5Repository: JanssenProject/jans
Length of output: 102
🏁 Script executed:
# Search for git history of the Fido2MetricsEntry file
git log --oneline --follow jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java 2>/dev/null | head -10Repository: JanssenProject/jans
Length of output: 125
🏁 Script executed:
# Check if there's a schema file that defines the jansFido2Metrics* object class
find . -type f \( -name "*.ldif" -o -name "*.schema" \) -exec grep -l "jansFido2MetricsEntry\|jansFido2Metrics" {} \;Repository: JanssenProject/jans
Length of output: 45
🏁 Script executed:
# Search for any documentation about Fido2MetricsEntry
find . -type f -name "*.md" -exec grep -l "Fido2MetricsEntry\|Fido2Metrics" {} \; 2>/dev/nullRepository: JanssenProject/jans
Length of output: 45
🏁 Script executed:
# Check the Fido2MetricsService to see how it's being used
rg "Fido2MetricsEntry" jans-fido2/server/src/main/java/io/jans/fido2/service/metric/ -A 5 -B 5 | head -80Repository: JanssenProject/jans
Length of output: 9526
🏁 Script executed:
# Look at the docuemts.ldif file (note the typo in the filename)
cat ./jans-linux-setup/jans_setup/templates/jans-fido2/docuemts.ldifRepository: JanssenProject/jans
Length of output: 2437
LDAP schema definition required: Add jansFido2MetricsEntry object class to LDAP schema.
This is a new metrics collection feature, not a breaking change to existing attributes. The jansFido2Metrics* prefixed attributes are newly introduced and have no existing persisted data to migrate. However, the LDAP schema definition for the jansFido2MetricsEntry object class and its attributes needs to be added to the schema files (e.g., jans-linux-setup/jans_setup/templates/jans-fido2/fido2.ldif) before deployment to ensure the LDAP directory properly validates and stores these entries.
🤖 Prompt for AI Agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java`
around lines 38 - 98, Add an LDAP schema definition for the new Fido2 metrics
model: create an objectClass named jansFido2MetricsEntry and define all
jansFido2Metrics* attributes used by the Fido2MetricsEntry class (e.g.,
jansFido2MetricsId, jansFido2MetricsType, jansFido2MetricsTimestamp,
jansFido2MetricsUserId, jansFido2MetricsUsername, jansFido2MetricsOperationType,
jansFido2MetricsStatus, jansFido2MetricsDuration,
jansFido2MetricsAuthenticatorType, jansFido2MetricsDeviceInfo,
jansFido2MetricsErrorReason, jansFido2MetricsErrorCategory,
jansFido2MetricsFallbackMethod, jansFido2MetricsFallbackReason,
jansFido2MetricsUserAgent, jansFido2MetricsIpAddress, jansFido2MetricsSessionId,
jansFido2MetricsAdditionalData, jansFido2MetricsNodeId,
jansFido2MetricsApplicationType) in the LDAP LDIF schema (the same template
location used for other FIDO2 schemas); specify appropriate syntaxes (e.g.,
distinguishedName/string/Date/Integer/JSON where supported), single- vs
multi-valued constraints, and include the new jansFido2MetricsEntry objectClass
in the appropriate structural/auxiliary category so instances of the
Fido2MetricsEntry Java class can be created and validated by the directory.
| @AttributeName(name = "jansFido2MetricsAdditionalData") | ||
| @JsonObject | ||
| private transient Map<String, Object> additionalData; |
There was a problem hiding this comment.
transient field with @JsonObject won't persist.
Same issue as in Fido2UserMetrics - the additionalData field is marked transient but has @JsonObject for ORM. This combination will prevent persistence of this field.
🔧 Suggested fix
`@AttributeName`(name = "jansFido2MetricsAdditionalData")
`@JsonObject`
- private transient Map<String, Object> additionalData;
+ private Map<String, Object> additionalData;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @AttributeName(name = "jansFido2MetricsAdditionalData") | |
| @JsonObject | |
| private transient Map<String, Object> additionalData; | |
| `@AttributeName`(name = "jansFido2MetricsAdditionalData") | |
| `@JsonObject` | |
| private Map<String, Object> additionalData; |
🤖 Prompt for AI Agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java`
around lines 90 - 92, The field additionalData in class Fido2MetricsEntry is
marked transient while annotated with `@JsonObject/`@AttributeName, so it will not
be persisted; remove the transient modifier (or otherwise make it non-transient)
on the additionalData field so the ORM can serialize it, ensuring the
`@JsonObject` annotation on additionalData is effective; update the declaration of
additionalData in Fido2MetricsEntry (matching the pattern used in
Fido2UserMetrics) and run tests to confirm persistence.
| @ApplicationScoped | ||
| @Path("/metrics") | ||
| public class Fido2MetricsController { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check authentication/authorization patterns in other controllers
echo "=== Checking for auth annotations in other controllers ==="
rg -n "@ProtectedApi|@RolesAllowed|@Authenticated|@SecurityRequirement" --type java jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/ | head -30
echo -e "\n=== Checking for security filters/interceptors ==="
rg -n "SecurityFilter|AuthenticationFilter|AuthorizationFilter|@Secured" --type java jans-fido2/server/ | head -20
echo -e "\n=== Looking for `@ProtectedApi` usage in the entire jans-fido2 ==="
rg -l "@ProtectedApi" --type java jans-fido2/server/src/ | head -10Repository: JanssenProject/jans
Length of output: 239
🏁 Script executed:
#!/bin/bash
# List actual controller files in jans-fido2
echo "=== Controller files in jans-fido2 ==="
fd -e java "Controller\.java$" jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/
# Check if there's a web.xml or security configuration
echo -e "\n=== Checking for web.xml or security configs ==="
fd -e xml "web.xml|application.xml|security" jans-fido2/server/
# Check for global filters or interceptors
echo -e "\n=== Checking for global filters/interceptors ==="
rg -n "Filter|Interceptor" --type java jans-fido2/server/src/main/java/io/jans/fido2/ -l | head -15
# Check OpenAPI/Swagger configuration for security definitions
echo -e "\n=== Checking for Swagger/OpenAPI config ==="
fd -e yaml -e yml "swagger|openapi" jans-fido2/ | head -10
# Check pom.xml for security-related dependencies
echo -e "\n=== Checking jans-fido2 pom.xml for security dependencies ==="
rg -n "security|auth" jans-fido2/server/pom.xml -i | head -20Repository: JanssenProject/jans
Length of output: 1609
🏁 Script executed:
#!/bin/bash
# Check AssertionController for security/auth patterns
echo "=== Checking AssertionController class structure ==="
head -50 jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/AssertionController.java
# Check web.xml for security constraints
echo -e "\n=== Checking web.xml for security config ==="
cat jans-fido2/server/src/main/webapp/WEB-INF/web.xml | grep -A 5 -B 5 "security\|filter\|constraint" | head -40
# Check Swagger config for security definitions
echo -e "\n=== Checking Swagger for security schemes ==="
head -100 jans-fido2/docs/jansFido2Swagger.yaml
# Check Fido2MetricsController endpoints
echo -e "\n=== Checking Fido2MetricsController methods ==="
rg -n "@GetMapping|@PostMapping|@Path|@RequestMapping" --type java jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java -A 2 | head -50Repository: JanssenProject/jans
Length of output: 7595
🏁 Script executed:
#!/bin/bash
# Check if Swagger has security definitions
echo "=== Looking for security definitions in Swagger spec ==="
rg -n "securitySchemes|security:|SecurityScheme" jans-fido2/docs/jansFido2Swagger.yaml | head -20
# Check the full Fido2MetricsController implementation
echo -e "\n=== Full Fido2MetricsController class ==="
wc -l jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
# Look at project README or documentation for API security info
echo -e "\n=== Checking README for security info ==="
fd -e md "README|SECURITY|readme" jans-fido2/ | head -5
# Check if there's any documentation about metrics endpoint security
echo -e "\n=== Checking for metrics security documentation ==="
rg -n "metrics.*security|security.*metrics" --type java jans-fido2/ -i | head -10
# Check AssertionController's POST endpoint to see full pattern
echo -e "\n=== Checking AssertionController POST endpoint pattern ==="
rg -n "@POST" jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/AssertionController.java -A 10 | head -30Repository: JanssenProject/jans
Length of output: 1979
🏁 Script executed:
#!/bin/bash
# Read the Fido2MetricsController file to understand its design
echo "=== Fido2MetricsController class overview ==="
head -100 jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
# Check README for security/deployment guidance
echo -e "\n=== jans-fido2 README content ==="
cat jans-fido2/README.md | head -80
# Check if there are servlet filters in web.xml
echo -e "\n=== Full web.xml to check for filters ==="
cat jans-fido2/server/src/main/webapp/WEB-INF/web.xmlRepository: JanssenProject/jans
Length of output: 5376
User-specific metrics endpoints should require authentication/authorization.
The endpoints like /entries/user/{userId} expose user activity data without visible authentication or authorization checks. While other controllers in jans-fido2 similarly lack explicit @ProtectedApi or @RolesAllowed annotations, user-specific metrics are particularly sensitive and should restrict access to authorized users or administrators only.
Ensure that authentication/authorization is enforced either:
- Via explicit annotations on the metrics controller, or
- Via documented infrastructure-level controls (API gateway, OAuth interceptor)
If security is enforced at deployment time, add documentation to clarify this.
🤖 Prompt for AI Agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`
around lines 37 - 39, Fido2MetricsController currently exposes user-specific
endpoints like /entries/user/{userId} without auth; update
Fido2MetricsController to enforce authorization by adding an appropriate
security annotation (e.g., `@ProtectedApi` or `@RolesAllowed`("ROLE_ADMIN") or
similar used across the project) on the class or on the user-specific method(s)
that handle /entries/user/{userId}, or if security is handled outside the app,
add a clear comment/Javadoc on Fido2MetricsController stating that
authentication/authorization is enforced by the API gateway/OAuth interceptor
and specify the required role/scope so reviewers know the endpoint is protected.
| // ISO formatter for UTC timestamps (aligned with FIDO2 services) | ||
| private static final DateTimeFormatter ISO_FORMATTER = DateTimeFormatter.ISO_LOCAL_DATE_TIME; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DateTime format assumption is documented but restrictive.
The formatter ISO_LOCAL_DATE_TIME rejects valid ISO-8601 strings with timezone offsets (e.g., 2024-01-01T00:00:00Z or 2024-01-01T00:00:00+00:00). This is intentional per the comment, but users familiar with ISO-8601 may be surprised.
Consider optionally supporting offset formats and converting to UTC, or adding API documentation noting that offsets are not accepted.
🤖 Prompt for AI Agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`
around lines 56 - 57, The ISO_FORMATTER currently set to
DateTimeFormatter.ISO_LOCAL_DATE_TIME in Fido2MetricsController rejects ISO-8601
strings with offsets; update the parsing/formatting to accept offset/zulu
timestamps by replacing or augmenting ISO_FORMATTER handling with an
offset-aware flow: attempt parsing with DateTimeFormatter.ISO_OFFSET_DATE_TIME
(or OffsetDateTime.parse) and convert the result to UTC (e.g., to Instant or
OffsetDateTime.withOffsetSameInstant(ZoneOffset.UTC) then to
LocalDateTime/desired format), and fallback to the existing ISO_LOCAL_DATE_TIME
parse if needed; ensure all places referencing ISO_FORMATTER or parsing
timestamps in Fido2MetricsController use this new accept-and-normalize logic and
update any API docs/comments to indicate offsets are supported.
| Map<String, Object> config = new HashMap<>(); | ||
| config.put("metricsEnabled", appConfiguration.isFido2MetricsEnabled()); | ||
| config.put("aggregationEnabled", appConfiguration.isFido2MetricsAggregationEnabled()); | ||
| config.put("retentionDays", appConfiguration.getFido2MetricsRetentionDays()); | ||
| config.put("deviceInfoCollection", appConfiguration.isFido2DeviceInfoCollection()); | ||
| config.put("errorCategorization", appConfiguration.isFido2ErrorCategorization()); | ||
| config.put("performanceMetrics", appConfiguration.isFido2PerformanceMetrics()); | ||
| config.put("supportedAggregationTypes", List.of( | ||
| Fido2MetricsConstants.HOURLY, | ||
| Fido2MetricsConstants.DAILY, | ||
| Fido2MetricsConstants.WEEKLY, | ||
| Fido2MetricsConstants.MONTHLY | ||
| )); | ||
|
|
||
| return Response.ok(dataMapperService.writeValueAsString(config)).build(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider defining response DTOs for better API documentation.
The /config endpoint (and others) returns Map<String, Object> which results in poor Swagger documentation. Based on learnings for this repository, API model classes should have property descriptions that are reflected in auto-generated Swagger specifications.
Consider creating dedicated response DTO classes (e.g., Fido2MetricsConfigResponse) with @Schema annotations for better API documentation:
`@Schema`(description = "FIDO2 metrics configuration")
public class Fido2MetricsConfigResponse {
`@Schema`(description = "Whether metrics collection is enabled")
private boolean metricsEnabled;
// ... other fields with descriptions
}This applies to other endpoints returning maps as well.
🤖 Prompt for AI Agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`
around lines 362 - 377, The controller returns a raw Map from the /config
endpoint in Fido2MetricsController which yields poor Swagger docs; create a new
DTO class (e.g., Fido2MetricsConfigResponse) with fields matching the map
entries (metricsEnabled, aggregationEnabled, retentionDays,
deviceInfoCollection, errorCategorization, performanceMetrics,
supportedAggregationTypes) and annotate each field with
`@Schema`(description="...") and proper types, then update the controller to
construct and return that DTO (instead of the Map and
dataMapperService.writeValueAsString(config)) so the endpoint method signature
exposes the concrete response type for OpenAPI generation.
| @GET | ||
| @Path("/health") | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| public Response getHealth() { | ||
| return processRequest(() -> { | ||
| Map<String, Object> health = new HashMap<>(); | ||
| health.put("status", "UP"); | ||
| health.put("metricsEnabled", appConfiguration.isFido2MetricsEnabled()); | ||
| health.put("aggregationEnabled", appConfiguration.isFido2MetricsAggregationEnabled()); | ||
| health.put("timestamp", ZonedDateTime.now(ZoneId.of("UTC")).toLocalDateTime().format(ISO_FORMATTER)); | ||
|
|
||
| return Response.ok(dataMapperService.writeValueAsString(health)).build(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Health check returns "UP" without verifying service health.
The health endpoint always returns "status": "UP" without checking if metricsService is actually functional or can connect to the database. This could be misleading in monitoring scenarios.
Consider adding a simple connectivity check or at minimum catching exceptions:
🔧 Suggested improvement
`@GET`
`@Path`("/health")
`@Produces`(MediaType.APPLICATION_JSON)
public Response getHealth() {
return processRequest(() -> {
Map<String, Object> health = new HashMap<>();
- health.put("status", "UP");
+ String status = "UP";
+ try {
+ // Verify service is operational (e.g., simple DB connectivity check)
+ metricsService.isOperational();
+ } catch (Exception e) {
+ status = "DEGRADED";
+ health.put("error", "Service check failed");
+ }
+ health.put("status", status);
health.put("metricsEnabled", appConfiguration.isFido2MetricsEnabled());
health.put("aggregationEnabled", appConfiguration.isFido2MetricsAggregationEnabled());
health.put("timestamp", ZonedDateTime.now(ZoneId.of("UTC")).toLocalDateTime().format(ISO_FORMATTER));
return Response.ok(dataMapperService.writeValueAsString(health)).build();
});
}🤖 Prompt for AI Agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`
around lines 385 - 398, The getHealth method currently always returns status
"UP"; update it to perform a simple connectivity check via the metricsService
(e.g., call a lightweight method like metricsService.isHealthy() or
metricsService.ping()/metricsService.testConnection()) and catch any exceptions
from that call, setting "status" to "DOWN" and including an error message when
the check fails; retain the existing fields (metricsEnabled, aggregationEnabled,
timestamp) and ensure you still return the serialized map via
dataMapperService.writeValueAsString(health) while handling exceptions so
failures produce an informative JSON health response instead of always "UP".
| private Response processRequest(RequestProcessor processor) { | ||
| try { | ||
| return processor.process(); | ||
| } catch (WebApplicationException e) { | ||
| // Re-throw web application exceptions as-is (they already have proper status codes) | ||
| throw e; | ||
| } catch (Exception e) { | ||
| // Log the full exception with stack trace for debugging | ||
| log.error("Error processing metrics request: {}", e.getMessage(), e); | ||
| // Wrap in a proper error response with context | ||
| String errorMessage = "An unexpected error occurred while processing the metrics request: " + e.getMessage(); | ||
| throw errorResponseFactory.unknownError(errorMessage); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider sanitizing error messages in responses.
Line 474 includes e.getMessage() directly in the error response. Exception messages may contain internal details (e.g., database connection strings, internal paths) that shouldn't be exposed to API clients.
🔧 Suggested improvement
} catch (Exception e) {
// Log the full exception with stack trace for debugging
log.error("Error processing metrics request: {}", e.getMessage(), e);
// Wrap in a proper error response with context
- String errorMessage = "An unexpected error occurred while processing the metrics request: " + e.getMessage();
+ String errorMessage = "An unexpected error occurred while processing the metrics request";
throw errorResponseFactory.unknownError(errorMessage);
}🤖 Prompt for AI Agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`
around lines 464 - 477, In processRequest
(Fido2MetricsController.processRequest) avoid including e.getMessage() in the
client-facing errorResponseFactory.unknownError; instead log the full exception
with log.error (already present) and pass a generic, non-sensitive message to
unknownError (e.g., "An unexpected error occurred while processing the metrics
request") or a sanitized summary; optionally generate and return an error
id/UUID to the client and include that id in the server log alongside the
exception to correlate logs (use that id in the unknownError payload) so
internal details never leak.
|
|
|
|
|
Description
Adds FIDO2 metrics system with real-time collection, automated aggregations, and 13 REST API endpoints for analytics (adoption, performance, devices, errors, trends). Includes cluster-safe scheduler and optimized JSON responses.
Target issue
Implements FIDO2 metrics REST API with collection, aggregation scheduler, and analytics endpoints for monitoring Passkey usage and performance
closes #11922
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.